fix(auth): cascade DELETE /auth/me to oauth_account + CORS on 500s - #45
Merged
Merged
Conversation
`DELETE /auth/me` 500s for any user with linked OAuth accounts.
SQLAlchemy's default ORM cascade ("save-update, merge") tries to
disassociate the children with `UPDATE oauth_account SET user_id =
NULL` before deleting the parent, which the `NOT NULL` on
`oauth_account.user_id` rejects. Adding `cascade="all, delete"` to
the `User.oauth_accounts` relationship makes the ORM emit DELETE
statements for the children first, so the parent delete succeeds.
Not using `passive_deletes=True` deliberately: it would skip the
ORM-side DELETEs and rely on the DB cascade, which is true in
prod (Postgres) but false in the SQLite test harness without
`PRAGMA foreign_keys=ON`, and would mask regressions.
The bug surfaced as a CORS error in the browser ("No
'Access-Control-Allow-Origin' header is present on the requested
resource") rather than a server error — because Starlette routes
`Exception` and `500` handlers to `ServerErrorMiddleware`, which
is hardcoded outside all user middleware and emits the 500 via the
outer ASGI `send`, bypassing `CORSMiddleware`. Registering an
`Exception` handler that re-derives the CORS headers and attaches
them to the response itself keeps the diagnostic info intact for
any future 500.
Both issues covered by regression tests in `test_delete_account.py`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DELETE /auth/mereturns 500 for any user with a linked OAuth account. Addingcascade="all, delete"toUser.oauth_accountsmakes the ORM delete children first instead of trying to NULL the FK.Access-Control-Allow-Originheader" instead of surfacing the real failure. A newExceptionhandler re-derives the CORS headers on the response itself.Root causes
Cascade.
User.oauth_accountshad nocascade=set, so SQLAlchemy's default (save-update, merge) fired onsession.delete(user). The ORM emittedUPDATE oauth_account SET user_id = NULL ...as a "disassociate children" step before the parent delete; theNOT NULLconstraint onoauth_account.user_idrejected it withNotNullViolationError, surfaced as a 500.Verified in Cloud Run logs for a real failing request:
Postgres'
ON DELETE CASCADEon the FK would handle this cleanly if the ORM stayed out of the way, but the ORM steps in first. Two ways to fix it:cascade="all, delete"(ORM emits explicit DELETEs for children) orpassive_deletes=True(ORM yields to the DB cascade). I went with just the first —passive_deletes=Truewould silently break the SQLite test harness, which defaults toforeign_keys=OFF. The slight perf cost (one DELETE per linked oauth_account row) is negligible for an account-deletion endpoint.CORS on 500. Starlette's
build_middleware_stacksplits exception handlers into two buckets:ServerErrorMiddlewareis the absolute outermost layer of any Starlette app — Starlette wires it that way deliberately. The 500 it emits travels through the outer ASGIsend, never the wrappedsendthatCORSMiddlewareuses to inject its headers. Result: every unhandled exception comes back to the browser without CORS headers, and the real failure (server-side bug) is hidden behind a generic CORS complaint.The fix re-derives the CORS headers (matching
cors_origin_list/cors_origin_regexfromapp/config.py) inside the exception handler and attaches them to the response. Same logicCORSMiddlewareruns, just emitted at the response level instead of the middleware level.Tests
tests/test_delete_account.pycovers both:test_delete_me_succeeds_for_user_with_linked_oauth— seeds a Steam-OAuth user + linkedoauth_accountrow, hitsDELETE /auth/me, asserts 204 and that both rows are gone. Reproduces the original 500 if the cascade is removed.test_unhandled_exception_response_includes_cors_headers— forces an uncaughtRuntimeErrorfrom a dependency, asserts the 500 carriesAccess-Control-Allow-OriginandAccess-Control-Allow-Credentials. UsesASGITransport(raise_app_exceptions=False)because Starlette'sServerErrorMiddlewarere-raises after sending the response (a debug-tooling convenience that httpx propagates by default).Test plan
DELETE /auth/mefrom the auth frontend — expect 204 and the toast clears